sqlite: add permission model checks to DatabaseSync#62957
Conversation
|
Review requested:
|
nodejs-github-bot
added
c++
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #62957 +/- ##
==========================================
- Coverage 90.21% 90.17% -0.05%
==========================================
Files 741 744 +3
Lines 241608 242218 +610
Branches 45534 45667 +133
==========================================
+ Hits 217972 218424 +452
- Misses 15154 15270 +116
- Partials 8482 8524 +42
🚀 New features to boost your workflow:
|
mcollina
force-pushed
the
sqlite-permission-checks
branch
from
8ffad93 to
877530d
Compare
RafaelGSS
left a comment
RafaelGSS
left a comment
There was a problem hiding this comment.
As long as it doesn't open precedence for reports targeting SQLite on permission model, I'm +1.
There was a problem hiding this comment.
Hmm, I think this needs to take into account https://sqlite.org/uri.html
- A database can be opened read-only with
mode=roorimmutable=1. - Another way to create an in-memory database is with
mode=memory.
Doing more string processing on the database path sounds a bit hacky though.
Edit: looks like we have not enabled SQLITE_USE_URI=1, but it would be good to leave a comment here that this needs to be updated when we do at the very least.
As long as it doesn't open precedence for reports targeting SQLite on permission model, I'm +1.
Passing a custom VFS to SQLite that can really prevent reads/writes might be a more comprehensive solution worth considering...
https://sqlite.org/vfs.html sounds really cool! I will take a look. |
Add permission model enforcement to DatabaseSync::Open(). File-backed databases now check kFileSystemRead (readOnly) or kFileSystemWrite (read-write) before calling sqlite3_open_v2. In-memory databases (:memory:) are exempt. Refs: https://hackerone.com/reports/3686625
Signed-off-by: Matteo Collina <matteo.collina@gmail.com>
Signed-off-by: Matteo Collina <matteo.collina@gmail.com>
mcollina
force-pushed
the
sqlite-permission-checks
branch
from
2bef788 to
b1cfecf
Compare
4 participants
Add permission model enforcement to
DatabaseSync::Open().kFileSystemRead(readOnly) orkFileSystemWrite(read-write) before callingsqlite3_open_v2.:memory:) are exempt.Known limitation:
db.exec()withATTACH DATABASESQL can still access arbitrary files at runtime. This would require parsing SQL to interceptATTACHstatements. Should we address that in this PR or a follow-up?